feat: add verifiable catalog approval provenance - #519
Merged
Conversation
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
imran-siddique
enabled auto-merge (squash)
August 17, 2026 02:42
imran-siddique
requested review from
AaronRoeF,
Qiang-Xu,
pforest and
podcastinator
August 17, 2026 16:10
pforest
approved these changes
Aug 17, 2026
This was referenced Aug 19, 2026
zohebk8s
pushed a commit
to zohebk8s/cmcp
that referenced
this pull request
Aug 20, 2026
The four tests that landed with agentrust-io#519 pass whether or not the policy, identity, and validity checks exist. Deleting the threshold count, the distinct-role guard, the role match, the principal and issuer match, or the interval order check leaves the suite green, so the module's M-of-N claim rested on code review alone. Added: * threshold shortfall, one approval against a 2-of-N policy * repeated role under a distinct-role policy, with distinct principals so the principal guard cannot mask it * principal, issuer, and role mismatches against the trusted key * validity boundaries, approved_at inclusive, expires_at exclusive, and an inverted interval * a genesis record at sequence 1, which fixes the all-zero previous_record_hash as the convention until the schema says otherwise * a schema and verifier agreement test over eleven malformed records, asserting that whatever the shipped schema rejects the verifier rejects too Each new test was checked by removing the guard it covers and confirming the test fails. The agreement test is the one that would have caught the bool timestamp divergence. One case is marked xfail strict: the schema sets a minimum of zero on approved_at and expires_at, the verifier does not, so a record with a negative approval timestamp verifies. That is left failing on purpose, since fixing it belongs with the decision on whether the verifier loads the schema at all. The schema is read from the repository at tests/../schemas, following test_trace_claim.py, because it is still absent from the wheel. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
imran-siddique
pushed a commit
that referenced
this pull request
Aug 21, 2026
…n fields (#517) (#531) * fix(catalog): pin the approval policy verifier-side and bind the chain fields verify_catalog_change read threshold, distinct_principals, and distinct_roles out of the record under verification, and checked policy_hash for digest shape only. A single trusted reviewer key could therefore issue a record declaring a threshold of one and have it verify, so the M-of-N property was unenforced. expected_policy_hash and expected_catalog_id are now required keyword arguments. The record's policy_hash must cover its own policy body and must equal the policy the verifier was configured with. compute_policy_hash defines that digest so producers and verifiers agree on it. previous_catalog_hash, sequence, and catalog_id were format checked and then unused. catalog_id is now always bound. expected_sequence and expected_previous_catalog_hash join expected_previous_record_hash as optional checkpoints, since those must come from an external pin. Also in this pass: * reject repeated principals and roles rather than counting distinct values, which admitted alice, bob, alice at a threshold of two * validate string and integer field types, so a non-hashable role raises CatalogApprovalError instead of escaping as TypeError * reject boolean timestamps, which the JSON Schema already rejected * validate the signature alphabet and decoded length, and move the decode out of the try block where its errors were masked as "signature is invalid" * drop the post-loop threshold checks, now unreachable Not addressed here: approvals are still judged against wall clock, so a record stops verifying once its approvals expire, and the JSON Schema is still not loaded by the verifier or shipped in the wheel. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * test(catalog): cover the approval checks the suite never asserted The four tests that landed with #519 pass whether or not the policy, identity, and validity checks exist. Deleting the threshold count, the distinct-role guard, the role match, the principal and issuer match, or the interval order check leaves the suite green, so the module's M-of-N claim rested on code review alone. Added: * threshold shortfall, one approval against a 2-of-N policy * repeated role under a distinct-role policy, with distinct principals so the principal guard cannot mask it * principal, issuer, and role mismatches against the trusted key * validity boundaries, approved_at inclusive, expires_at exclusive, and an inverted interval * a genesis record at sequence 1, which fixes the all-zero previous_record_hash as the convention until the schema says otherwise * a schema and verifier agreement test over eleven malformed records, asserting that whatever the shipped schema rejects the verifier rejects too Each new test was checked by removing the guard it covers and confirming the test fails. The agreement test is the one that would have caught the bool timestamp divergence. One case is marked xfail strict: the schema sets a minimum of zero on approved_at and expires_at, the verifier does not, so a record with a negative approval timestamp verifies. That is left failing on purpose, since fixing it belongs with the decision on whether the verifier loads the schema at all. The schema is read from the repository at tests/../schemas, following test_trace_claim.py, because it is still absent from the wheel. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * docs(changelog): record the catalog approval policy pinning fix Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * test(catalog): point the xfail at #533 Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * test(catalog): cover the three branches the suite still missed Codecov flagged one line of the diff: the policy distinctness flags guard, which nothing exercised. Three more lines in the module were uncovered before this PR and are cheap to reach from the same fixture, so they go with it. * a policy flag set to a truthy string rather than a boolean * an unknown profile * a digest of the wrong shape as well as the wrong alphabet * an approval carrying a stray member * a one character signature, which is the only input that reaches the base64 decode error path, since anything with a bad alphabet is rejected before it and anything longer decodes and fails on length The module is now at 100 percent line coverage, 28 tests and the deliberate xfail. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * test(catalog): carry the JCS round trip onto the new verifier signature #532 landed first, and its end to end test calls verify_catalog_change, which this branch gives two required keyword arguments. Left alone the test fails with a TypeError on main once this merges, so the fix belongs here rather than in a follow-up. The record it builds is unchanged apart from computing policy_hash with compute_policy_hash, which is what the verifier now requires the field to cover. The point of the test is unaffected: a non-ASCII reviewer identity still signs and verifies end to end. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * fix(catalog): count a reviewer key once, whatever the policy says The M-of-N property still had a hole that the distinctness rules do not close, because nothing bound approvals to distinct keys. Both of these verified before this change: * threshold 3 with distinct_principals false, one key signing the identical approval three times, accepted with valid_approvals 3 * threshold 2 with distinct_roles true, one key signing twice as "security" and "owner", accepted, because a TrustedReviewer with role None lets the record assert whatever role it likes The first is arguably what the policy asked for, but a repeated signature is one approval presented three times rather than three approvals. The second is not what the policy asked for at all: it demands two roles and got one key. A key now counts once per record, checked after the principal and role rules so their errors keep reporting the more specific cause. Verified by removing the guard: 2 of the 31 tests fail. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * docs(changelog): record the reviewer key reuse rule Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> * docs(spec): say that a key identifier must name one key Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> --------- Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com> Co-authored-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nnImplements #517 as a detached, signed catalog-change provenance record.nn- Adds versioned catalog approval schema and threat-boundary documentation.n- Uses Ed25519 signatures over the record and each approval; trust keys stay verifier-side.n- Enforces M-of-N approvals, distinct principals/roles, issuer and key binding, expiry, revocation, and chain checkpoints.n- Binds new_catalog_hash to the runtime catalog hash.n- Adds deterministic focused vectors for tampering, stale chain, revocation, expiry, and duplicate principals.nn## Evidencenn- Focused pytest: 4 passedn- Ruff: passedn- mypy approval module: passedn- JSON Schema meta-validation: passedn- git diff --check: passedn- Full local suite is environment-blocked at collection by pre-existing missing dependencies: agentrust_trace, agent_os, cedarpy, and mcp.n`nSigned-off-by: Imran Siddique imran@agentrust.com